-
Notifications
You must be signed in to change notification settings - Fork 780
Save local set data in EndTryTable #2673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5f532d5 to
3071dba
Compare
|
Ok try tables use a very different mechanism compared to normal blocks. I was able to trigger the issue in #2670 again with adding a local ref. I checked the push / pop labels then. It looks like the BeginTryTable does not construct any labels. Probably because no code is generated then. The label construction happens at EndTryTable. Then the expressions are visited, and OnEnd is called by EndTryTableExpr. I don't know how the interpreter handles this, probably using its I would need some help to understand how this code should work. Maybe more complex test cases are needed. |
c5fc954 to
07f49d2
Compare
|
If the code block of a try table is between |
91593bf to
68a30e1
Compare
68a30e1 to
c0dbe51
Compare
| out/test/parse/bad-refs-in-trytable.txt:16:17: error: uninitialized local reference | ||
| local.get 2 | ||
| ^ | ||
| out/test/parse/bad-refs-in-trytable.txt:21:15: error: uninitialized local reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this is test that is missing from the upstream repo? Perhaps we should file an issue there, with the hope that can one day remove this downstream test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand this question. I have created this test myself. The test checks that the "is set" bitset is correctly working. My code was wrong, I saved the bitset in a wrong place. I misunderstood the API sequence order for try_tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you might refer to the failure. That is intentional, the WebAssembly set-before-use check is very simple.
The spec test interpreter also yields this error (although stops at the first error):
test.wast:13.17-13.18: validation error: uninitialized local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is basically this: If this is an interesting test case, why can't it be (or shouldn't it be) upstream in the spec repo. I guess this is question we should be asking of all of our local/downstream tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case this change LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are interested, why not. Anyway, it is possible that a similar test is already present in the spec test repo. The GC/function-references proposal don't have such test, since (reworked) exceptions is another proposal, but the full WebAssembly 3.0 test system may have.
I have checked with
gdbthatOnEndis really called. I hope I don't make more mistakes. I have also added the test case withoutunreachable, since theunreachableshould not affect the bug, although it might trigger future bugs, so both should be present.